Repository navigation
fix: assistant: clear saved state on sign-out, and strip ?sessionId= after a failed open (#1999) - #2005
fix: assistant: clear saved state on sign-out, and strip ?sessionId= after a failed open (#1999)#2005frano-m wants to merge 1 commit into
?sessionId= after a failed open (#1999)#2005Conversation
…` after a failed open (#1999) `useAssistantChat` now derives `isSaved` and `savedAnalysisId` from auth, so a save, reply or restore that lands after sign-out can't put either back. `AuthProvider.logout` signs out on the page before the logout request, so no account-scoped request races the session being ended. A non-retryable 4xx on a restore whose session came from `?sessionId=` strips the param, so a reload no longer fails the same way. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: cc-claude-tools <cc-claude-tools@clevercanary.com>
There was a problem hiding this comment.
🟢 Approval recommended
There are no active review issues for the sign-out state and session-restore URL cleanup changes.
No open issues 🎉
What changed in this PR
Addresses #1999 by preventing stale saved-conversation indicators after sign-out and repeated failed restores from assistant session URLs.
Changes:
- Derives saved-state indicators from authentication and clears the local user before the logout request completes.
- Removes
?sessionId=after non-retryable restore failures using the latest router state, while preserving it for retryable failures. - Adds regression tests for sign-out timing, late responses, and restore URL cleanup.
| File | Description |
|---|---|
tests/providers/authenticationProvider.test.tsx |
Verifies local sign-out occurs before the logout request completes. |
tests/hooks/useAssistantChat.restore.test.ts |
Tests URL cleanup for permanent failures and preservation for transient failures. |
tests/hooks/useAssistantChat.autosave.test.ts |
Tests saved-state clearing on sign-out and after late restore or reply responses. |
packages/shared/views/AssistantView/hooks/UseAssistantChat/types.ts |
Documents signed-out behavior for saved-state fields. |
packages/shared/views/AssistantView/hooks/UseAssistantChat/hook.ts |
Gates saved-state indicators on authentication and clears failed session URLs. |
packages/shared/providers/authentication/provider.tsx |
Clears the local user before sending the logout request. |
Review effort: Balanced
NoopDog
left a comment
NoopDog
left a comment
There was a problem hiding this comment.
Thanks, deriving isSaved from auth on each render is cleaner than the old sign-out effect, and moving setUser(null) ahead of the logout POST closes the real gap. The URL strip and its retryable-failure test look right too. Two required points, both about making the code and #1999 say the same thing:
- Required: the
savedAnalysisIdgate and its test protect a value nothing reads. See the inline comment athook.ts:650. - Required: make what shipped match #1999, or update #1999. Two places differ:
- #1999's second done item says a save that resolves after sign-out must not call
setLastSave.hook.ts:364still calls it. The PR's reasoning holds: the History list is the only reader, it isn't rendered while signed out (chatHistorySelector.tsx:32), and signing in is a full-page redirect. So either gatesetLastSaveas the issue asks, or edit #1999's done item to record that the logout reorder covers it instead. - "How to verify" step 3, in both the PR and #1999, says a reload after a refused open shows an empty assistant. That's only true when nothing was on screen before the open. If an earlier conversation was showing, its id is still in localStorage (it's only cleared when it matches the failed id), so a reload restores that conversation. Either change the code to match, or reword step 3 in both places.
- #1999's second done item says a save that resolves after sign-out must not call
- Optional: no test covers the "auth loading" and "login not configured" parts of
isSignedOut(hook.ts:159). Dropping!isAuthLoadingalone, orisConfigured &&alone, leaves every test passing. Consider a test with auth configured and still loading, where a restore returnssaved: true, assertingisSavedstays true. - Optional: no test shows the
routerRefupdate is needed (hook.ts:151-154). DeletingrouterRef.current = routerleaves the URL tests passing, because the mocked router'squerynever changes after mount. Consider having the mockedreplaceupdate the query, or adding a second parameter after mount, and asserting that the strip keeps it.
| logan, | ||
| messages, | ||
| savedAnalysisId, | ||
| savedAnalysisId: isSignedOut ? null : savedAnalysisId, |
There was a problem hiding this comment.
Required: by the same reasoning the PR gives for lastSave, nothing reads savedAnalysisId while signed out. Its only reader is the History list, which shows the sign-in prompt instead of itself (chatHistorySelector.tsx:32). So this gate, its doc line in types.ts, and the test "signing out leaves no saved analysis showing as open" (useAssistantChat.autosave.test.ts:554) protect a value nobody sees. It also leaves lastSave ungated and savedAnalysisId gated, which is inconsistent. Consider dropping the gate, the doc line and that test, and updating #1999's first done item to match, since #1999 asked for this. The isSaved gate one line up is needed: ChatPanel shows that label whether or not the user is signed in.
Closes #1999
What changed
useAssistantChatderivesisSavedandsavedAnalysisIdfrom auth: both read asfalse/nullwhile the viewer is signed out. Any save, reply or conversation load that lands after sign-out now shows as unsaved, with no highlighted History row. Before this change, a reply or load still in flight at sign-out brought "Saved to your account" back next to the sign-in prompt. The sign-out effect that cleared these values one at a time is removed; it had nothing left to do.AuthProvider.logoutnow callssetUser(null)before the/auth/logoutPOST, not after it. Every account-scoped reader (for example the History list) stops as soon as the viewer clicks sign out, and doesn't send one more request against a session the server is ending.?sessionId=now strips the param. That covers both an open from History and a page loaded from a?sessionId=link. Retryable failures (no response, 5xx, 408, 429) keep it. The strip reads the router through a ref, so it builds on the current URL rather than the one from when the callback was created.Why
Issue #1999 lists three edge cases in
useAssistantChat. Review found that two of them could not happen as the issue described: the History list, the only reader ofsavedAnalysisIdandlastSave, unmounts as soon asisAuthenticatedgoes false. Review also found two real gaps nearby:logout's POST, before the user is cleared.This PR fixes those at their source instead of guarding each request inside the hook. The
?sessionId=case was a real bug: every reload showed "That conversation is no longer available" until the user started a new analysis.Assumptions I made
isSignedOutonly applies when login is configured and auth has finished loading. While auth is loading, the label shows what the server reported, as it did before.isNamedalways got its session from?sessionId=, either on page load or because an open wrote it there. A restore from the localStorage pointer leaves the URL alone.lastSaveitself is not gated. Its only reader unmounts on sign-out, and the logout ordering closes the window where it could still trigger a request.How to verify
Run the site locally with the backend and login enabled.
Definition of done from the issue:
savedAnalysisId. Sign in, open a saved conversation from History, then sign out from the account menu. History is replaced by the sign-in prompt, the panel no longer says "Saved to your account", and when you sign back in no stale row is selected.setLastSave. In DevTools, throttle the network to "Slow 3G". Sign in on a conversation with unsaved messages, then sign out while the save request is pending. In the Network tab, nosaved-analysesrequest is sent after the/auth/logoutPOST, and the panel does not show "Saved to your account".?sessionId=; retryable failures leave it alone. Open a saved conversation whose session the server refuses. One way: in DevTools, block the/assistant/restorerequest URL, or use a session id that no longer exists. The URL loses?sessionId=, and a reload shows an empty assistant with no "no longer available" error. Also load/assistant?sessionId=<missing id>directly: you see the error once, the param is gone, and a reload is clean.tests/hooks/useAssistantChat.autosave.test.ts: "signing out leaves no saved analysis showing as open", "a conversation that loads after signing out does not show as saved", "a reply that lands after signing out does not show as saved".tests/hooks/useAssistantChat.restore.test.ts: "a 404 on a session named in the URL drops it from the URL", "a conversation refused after an open is dropped from the URL", "a conversation that fails to load after an open for a transient reason stays in the URL", plus a no-strip assertion for a stored pointer.tests/providers/authenticationProvider.test.tsx: "signs out on the page before the server has ended the session".npx jest tests/hooks/useAssistantChat tests/providers. The regression tests fail onmain.Note:
bucketLabelintests/views/queueStatsView.test.tsxfails locally onmaintoo. It is unrelated to this change.🤖 Generated with Claude Code
🐦 Built with cc-claude-tools